Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure transaction ID is always unique #3190

Merged
merged 5 commits into from
Nov 2, 2018
Merged

Ensure transaction ID is always unique #3190

merged 5 commits into from
Nov 2, 2018

Conversation

naegelin
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Transaction ID implicitly suggests that this is a unique identifier per transaction. However in the case of a refresh / re-use of an adunit the transaction ID remains the same causing undesirable bidding with certain SSPs.

Transaction ID implicitly suggests that this is a unique identifier per transaction. However in the case of a refresh / re-use of an ad unit the transaction ID remains the same causing undesirable bidding with certain SSPs.
@naegelin
Copy link
Contributor Author

naegelin commented Oct 17, 2018

linter is failing using the standard github inline editor. Leaving this as is for the time being .

@jsnellbaker
Copy link
Collaborator

@naegelin The Prebid.js project uses the gulp-eslint plugin to maintain the styling rules for the project. Can you please make the necessary updates in your file to conform to the linting rules? You can locally check the linting status/errors by running gulp lint in your Prebid.js project.

@naegelin
Copy link
Contributor Author

The tests for transaction id validation are now failing (obviously). I'm leaving this as is for discussion first @mkendall07 @jsnellbaker as a decision needs to be made regarding what the intended behavior should actually be here.

@mkendall07
Copy link
Member

It looks like this was changed with #2706
Including @snapwich here for comment. I think we do want to always assign a new transactionId though.

@mkendall07 mkendall07 requested a review from snapwich October 22, 2018 19:25
@snapwich
Copy link
Collaborator

I'm not sure on what the business logic of transactionId should be, maybe @bretg has input?

In #2706 I specifically tried to copy the existing business logic of having a persistent transactionId assigned to each adUnit. Before #2706, transactionIds were added when you called pbjs.addAdUnit() but they were not refreshed if you were to pbjs.requestBids() multiple times. I kept that same business logic in #2706, however I moved the transactionId creation inside of pbjs.requestBids() to ensure that all adUnits had a transactionId, including ones passed directly to pbjs.requestBids() without the use of pbjs.addAdUnit().

@mkendall07
Copy link
Member

ah interesting, thanks Rich. @jaiminpanchal27 do you have any idea on this?

@bretg
Copy link
Collaborator

bretg commented Oct 25, 2018

The definition of transactionID is in http://prebid.org/dev-docs/bidder-adaptor.html#bidrequest-parameters:

Transaction ID is unique for each ad unit with a call to requestBids, but same across bidders. This is the ID that DSPs need to recognize the same impression coming in from different supply sources.

So if transactionId is not currently changing on a refresh, I agree with @naegelin that this is a bug.

@snapwich
Copy link
Collaborator

So the only other concern I could possibly think of with this pull-request was if an implementor wanted to generate their own transactionIds and pass them through then this would overwrite their ids. Is that something that people do? @bretg I know we generated our own transactionIds as part of hpv2, but I can't remember if that was just a temporary fix because #2706 hadn't happened yet.

If it's no big deal then this LGTM. (other than the tests which need to be updated as well).

@bretg
Copy link
Collaborator

bretg commented Oct 26, 2018

I think we found that transaction ID wasn't being added because we weren't using pbjs.addAdUnit. Our code checks:

                    if (!adUnit.transactionId) {
                        adUnit.transactionId = generateUUID();
                    }

So yes - I think #2706 should have resolved that issue. So I think we're good here. Except that the unit tests are failing.

@mkendall07
Copy link
Member

@naegelin
Please update the test and we can merge this. Thanks!

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@harpere harpere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@snapwich snapwich merged commit 033b151 into prebid:master Nov 2, 2018
@naegelin
Copy link
Contributor Author

naegelin commented Nov 6, 2018

Thanks for updating guys - was on vacation and just saw this :)

idettman pushed a commit to rubicon-project/Prebid.js that referenced this pull request Nov 14, 2018
* Ensure transaction ID is always unique

Transaction ID implicitly suggests that this is a unique identifier per transaction. However in the case of a refresh / re-use of an ad unit the transaction ID remains the same causing undesirable bidding with certain SSPs.
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* Ensure transaction ID is always unique

Transaction ID implicitly suggests that this is a unique identifier per transaction. However in the case of a refresh / re-use of an ad unit the transaction ID remains the same causing undesirable bidding with certain SSPs.
pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019
* Ensure transaction ID is always unique

Transaction ID implicitly suggests that this is a unique identifier per transaction. However in the case of a refresh / re-use of an ad unit the transaction ID remains the same causing undesirable bidding with certain SSPs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants